Added support for TencentOS and OpenCloudOS#1926
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThe PR broadens RPM OS detection in the generator and setup scripts to recognize TencentOS and OpenCloudOS, and updates RPM CI jobs with matching container images. The ChangesRPM detection and CI coverage
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
scripts/rpm/setup_25.x (1)
1-40: Consider consolidating shared logic into a reusable helper.The RPM detection block, logger function, error handler, and command_exists function are duplicated across all
setup_*.xscripts. As new distributions like TencentOS/OpenCloudOS are added, each script must be updated individually (as this PR demonstrates with 8+ files).Consider extracting these common functions into a shared sourced script (e.g.,
common.sh) or using a template generator to reduce maintenance burden and ensure consistency.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/rpm/setup_25.x` around lines 1 - 40, Extract the duplicated helpers (log, handle_error, command_exists) and the RPM detection block into a shared script (e.g., common.sh) and have each setup_*.x script source that file at the top; specifically move the functions log, handle_error, command_exists and the RPM detection if-block into common.sh, export any needed environment/config variables (or return non-zero on unsupported OS) and replace the in-file copies with a single "source common.sh" line, then remove the duplicated blocks from setup_25.x and the other setup_*.x scripts so future distro additions (TencentOS/OpenCloudOS) only need to update common.sh.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/rpm/script_generator/base_script.sh`:
- Around line 35-38: The conditional guard beginning with "if ! [ -f
/etc/redhat-release ] && ! grep -q \"Amazon Linux\" /etc/system-release ..."
misses fallbacks that check /etc/os-release for TencentOS/OpenCloudOS; update
that if-condition to also grep /etc/os-release (e.g., add "&& ! grep -qi
\"TencentOS\" /etc/os-release 2>/dev/null && ! grep -qi \"OpenCloudOS\"
/etc/os-release 2>/dev/null") so the template recognizes those distros when only
/etc/os-release is present, then regenerate the derived setup_*.x files from the
generator so the change propagates.
In `@scripts/rpm/setup_16.x`:
- Around line 107-110: The guard in the if condition that checks for non-RPM
systems only inspects /etc/system-release and /etc/redhat-release but misses
distributions identified in /etc/os-release; update the conditional around the
multi-line if (the block starting with "if ! [ -f /etc/redhat-release ] \") to
also check /etc/os-release for the same strings ("Amazon Linux", "TencentOS",
"OpenCloudOS") using grep -q /etc/os-release 2>/dev/null (case-insensitively
where used) so machines identified via /etc/os-release are correctly detected
and the RPM-system check behaves the same as newer scripts.
- Around line 69-73: The release-status lines in scripts/rpm/setup_16.x are
stale (the URL/status labels for setup_18.x, setup_20.x, setup_21.x, etc.);
update each URL line (e.g., the literal lines containing
"https://rpm.nodesource.com/setup_16.x",
"https://rpm.nodesource.com/setup_18.x",
"https://rpm.nodesource.com/setup_19.x",
"https://rpm.nodesource.com/setup_20.x",
"https://rpm.nodesource.com/setup_21.x") to reflect the current Node.js release
matrix as of 2026-03-09: mark 18.x and 21.x as EOL, mark 20.x and 22.x as
Maintenance LTS, add or mark 24.x as Active LTS and 25.x as Current, and adjust
colors/annotations (red/green/bold) accordingly so the recommended installer
points to the current LTS (24.x) or Current (25.x) as intended.
---
Nitpick comments:
In `@scripts/rpm/setup_25.x`:
- Around line 1-40: Extract the duplicated helpers (log, handle_error,
command_exists) and the RPM detection block into a shared script (e.g.,
common.sh) and have each setup_*.x script source that file at the top;
specifically move the functions log, handle_error, command_exists and the RPM
detection if-block into common.sh, export any needed environment/config
variables (or return non-zero on unsupported OS) and replace the in-file copies
with a single "source common.sh" line, then remove the duplicated blocks from
setup_25.x and the other setup_*.x scripts so future distro additions
(TencentOS/OpenCloudOS) only need to update common.sh.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2f8e8f6f-9477-4b25-98f5-d76e085486a9
📒 Files selected for processing (11)
scripts/rpm/script_generator/base_script.shscripts/rpm/setup_16.xscripts/rpm/setup_18.xscripts/rpm/setup_20.xscripts/rpm/setup_21.xscripts/rpm/setup_22.xscripts/rpm/setup_23.xscripts/rpm/setup_24.xscripts/rpm/setup_25.xscripts/rpm/setup_current.xscripts/rpm/setup_lts.x
|
Hi @shouhuanxiaoji , This PR is looking good, thanks for the interest to contribute on the project. |
I have tried to add support for OS in the CI code, but opencloudos only has minimal support |
|
the same issue on OpenCloudOS. The installation script fails to detect it as an RPM-based system. This PR fixes it for me. Would love to see this merged？ |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/ci.yaml:
- Line 109: The rpm job matrix is missing OpenCloudOS coverage, so update the os
list in the CI workflow’s rpm job to include an OpenCloudOS container alongside
TencentOS. Use the existing rpm-minimal job’s OpenCloudOS image as a reference,
and ensure the full rpm job in the workflow covers the non-minimal OpenCloudOS
path by adding the appropriate opencloudos/opencloudos9 image entry.
- Line 109: The rpm workflow matrix includes tencentos/tencentos4:latest, but
this image does not provide dnf, so the package install step in the CI job will
fail. Update the matrix entry in the CI workflow to either use a TencentOS image
that supports dnf or adjust the rpm job logic to use the package manager
supported by the tencentos/tencentos4:latest leg, referencing the os matrix and
the dnf install/update steps in the workflow.
- Line 201: The rpm-minimal matrix in the CI workflow includes images that do
not ship microdnf, causing the install step to fail. Update the matrix in the
workflow job to remove or replace tencentos/tencentos4-minimal:latest and
opencloudos/opencloudos9-minimal:latest with images that provide microdnf or
dnf, or exclude those entries from this job. Keep the change localized to the
matrix definition so the rpm-minimal job only runs on supported base images.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 89dea634-d4f7-4148-a930-16f40b83544e
📒 Files selected for processing (13)
.github/workflows/ci.yamlscripts/rpm/script_generator/base_script.shscripts/rpm/setup_16.xscripts/rpm/setup_18.xscripts/rpm/setup_20.xscripts/rpm/setup_21.xscripts/rpm/setup_22.xscripts/rpm/setup_23.xscripts/rpm/setup_24.xscripts/rpm/setup_25.xscripts/rpm/setup_26.xscripts/rpm/setup_current.xscripts/rpm/setup_lts.x
✅ Files skipped from review due to trivial changes (1)
- scripts/rpm/setup_lts.x
🚧 Files skipped from review as they are similar to previous changes (10)
- scripts/rpm/setup_18.x
- scripts/rpm/setup_20.x
- scripts/rpm/setup_23.x
- scripts/rpm/setup_25.x
- scripts/rpm/setup_24.x
- scripts/rpm/script_generator/base_script.sh
- scripts/rpm/setup_current.x
- scripts/rpm/setup_16.x
- scripts/rpm/setup_22.x
- scripts/rpm/setup_21.x
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/ci.yaml (1)
282-283: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Install NSolidstep inrpm-minimalstill hardcodesmicrodnfinstead of$PM.The
install gitstep (lines 210–219) detects the package manager and exports it asPM, and theAudit Repository/Install Nodejssteps correctly use${{ env.PM }}. However, thisInstall NSolidstep was missed and still callsmicrodnfdirectly. For matrix entries where the detected PM isdnforyum(e.g.,tencentos/tencentos4-minimal:latest,opencloudos/opencloudos9-minimal:latest), this step will fail.Proposed fix
- name: Install NSolid if: matrix.version != 25 && matrix.version != 26 run: | - microdnf remove nodejs -y - microdnf install nsolid -y + ${{ env.PM }} remove nodejs -y + ${{ env.PM }} install nsolid -y🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/ci.yaml around lines 282 - 283, The Install NSolid step in the rpm-minimal workflow is still hardcoded to use microdnf, unlike the earlier package-manager detection flow that exports PM. Update that step to use the same PM environment variable as the Audit Repository and Install Nodejs steps, so the package removal/install commands in the workflow work correctly for dnf and yum-based matrix images as well.
♻️ Duplicate comments (2)
.github/workflows/ci.yaml (2)
109-109: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
tencentos/tencentos4:latestmay lackdnf—rpmjob hardcodesdnfeverywhere.Previously flagged and still unaddressed. The
rpmjob usesdnfat lines 118–119, 128, 132, and 182–183 without package-manager detection. Iftencentos/tencentos4:latestdoesn't shipdnf, every step afterinstall gitwill fail. Consider either using a TencentOS image that includesdnfor adding the same PM-detection logic used in therpm-minimaljob.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/ci.yaml at line 109, The rpm job still hardcodes dnf in multiple steps, which breaks on tencentos/tencentos4:latest if that image does not include dnf. Update the rpm job in the workflow to either use a TencentOS image with dnf available or add package-manager detection like the rpm-minimal job, and apply that logic consistently in the install and update steps currently using dnf.
109-109: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winOpenCloudOS still missing from the
rpmjob matrix.This was previously flagged and remains unresolved. The
rpm-minimaljob includesopencloudos/opencloudos9-minimal:latest, but the fullrpmjob only hastencentos/tencentos4:latest. Addopencloudos/opencloudos9:latestto ensure the non-minimal setup path is also tested.Proposed fix
- os: [ "fedora:41", "amazonlinux:2023", "rockylinux:9", "rockylinux:8", "redhat/ubi9:latest", "tencentos/tencentos4:latest" ] + os: [ "fedora:41", "amazonlinux:2023", "rockylinux:9", "rockylinux:8", "redhat/ubi9:latest", "tencentos/tencentos4:latest", "opencloudos/opencloudos9:latest" ]🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/ci.yaml at line 109, The rpm job matrix is missing the full OpenCloudOS image, so the non-minimal RPM path is not covered. Update the matrix in the CI workflow’s rpm job to include opencloudos/opencloudos9:latest alongside the existing entries, using the same matrix structure as the rpm-minimal/opencloudos coverage. Keep the change localized to the os list used by the rpm job so the setup path is tested without altering other job behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In @.github/workflows/ci.yaml:
- Around line 282-283: The Install NSolid step in the rpm-minimal workflow is
still hardcoded to use microdnf, unlike the earlier package-manager detection
flow that exports PM. Update that step to use the same PM environment variable
as the Audit Repository and Install Nodejs steps, so the package removal/install
commands in the workflow work correctly for dnf and yum-based matrix images as
well.
---
Duplicate comments:
In @.github/workflows/ci.yaml:
- Line 109: The rpm job still hardcodes dnf in multiple steps, which breaks on
tencentos/tencentos4:latest if that image does not include dnf. Update the rpm
job in the workflow to either use a TencentOS image with dnf available or add
package-manager detection like the rpm-minimal job, and apply that logic
consistently in the install and update steps currently using dnf.
- Line 109: The rpm job matrix is missing the full OpenCloudOS image, so the
non-minimal RPM path is not covered. Update the matrix in the CI workflow’s rpm
job to include opencloudos/opencloudos9:latest alongside the existing entries,
using the same matrix structure as the rpm-minimal/opencloudos coverage. Keep
the change localized to the os list used by the rpm job so the setup path is
tested without altering other job behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5ef45362-f0cc-4456-a13d-6db43a38a0c2
📒 Files selected for processing (2)
.github/workflows/ci.yamlscripts/rpm/setup_26.x
🚧 Files skipped from review as they are similar to previous changes (1)
- scripts/rpm/setup_26.x
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
.github/workflows/ci.yaml (1)
109-109: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winOpenCloudOS still missing from the
rpmjob matrix.This was flagged in a previous review and remains unaddressed. The
rpm-minimaljob (line 201) includesopencloudos/opencloudos9-minimal:latest, but the fullrpmjob only addstencentos/tencentos4:latest. Since the PR objective is to add support for both TencentOS and OpenCloudOS, the fullrpmjob should also test an OpenCloudOS container to cover the non-minimal setup path.Proposed fix
- os: [ "fedora:41", "amazonlinux:2023", "rockylinux:9", "rockylinux:8", "redhat/ubi9:latest", "tencentos/tencentos4:latest" ] + os: [ "fedora:41", "amazonlinux:2023", "rockylinux:9", "rockylinux:8", "redhat/ubi9:latest", "tencentos/tencentos4:latest", "opencloudos/opencloudos9:latest" ]🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/ci.yaml at line 109, The rpm job matrix in the CI workflow still omits OpenCloudOS, so the full rpm path is not covered. Update the os matrix for the rpm job alongside the existing entries in the workflow so it also includes the OpenCloudOS container used by rpm-minimal, keeping TencentOS and the other current images intact. Locate the matrix under the rpm job in the workflow and add the OpenCloudOS image there as well.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/ci.yaml:
- Line 109: The rpm job still hardcodes dnf in its package steps, but the matrix
now includes tencentos/tencentos4:latest which does not ship dnf. Update the rpm
job to use the same dynamic package-manager detection pattern already used in
rpm-minimal, and apply it consistently in the install/update/repo/nodejs/nsolid
steps within the rpm workflow. If you prefer not to add detection, replace
tencentos/tencentos4:latest with an image that provides dnf.
---
Duplicate comments:
In @.github/workflows/ci.yaml:
- Line 109: The rpm job matrix in the CI workflow still omits OpenCloudOS, so
the full rpm path is not covered. Update the os matrix for the rpm job alongside
the existing entries in the workflow so it also includes the OpenCloudOS
container used by rpm-minimal, keeping TencentOS and the other current images
intact. Locate the matrix under the rpm job in the workflow and add the
OpenCloudOS image there as well.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6750a336-712a-458c-b4c3-7d6e083eee67
📒 Files selected for processing (2)
.github/workflows/ci.yamlscripts/rpm/setup_26.x
🚧 Files skipped from review as they are similar to previous changes (1)
- scripts/rpm/setup_26.x
4 participants
OpenCloudOS is a derivative distribution of TencentOS, so the description in this article applies to both distributions.
TencentOS use rpm as package management, but it has not /etc/os-release, only has /etc/system-release and /os-release, so added a patch to support.
Summary by CodeRabbit